Skip to content

Fix useConfirm host element leaking into the DOM and tests#7935

Open
Copilot wants to merge 5 commits into
mainfrom
copilot/fix-useconfirm-leak
Open

Fix useConfirm host element leaking into the DOM and tests#7935
Copilot wants to merge 5 commits into
mainfrom
copilot/fix-useconfirm-leak

Conversation

Copilot AI commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

useConfirm() / confirm() rendered ConfirmationDialog into a detached createRoot whose host <div>, appended to document.body, was never removed on close. The empty host accumulated across calls and leaked into consumers—most visibly poisoning subsequent @testing-library/react tests whose cleanup() cannot reach a sibling root.

This is the surgical, non-breaking subset of the issue's proposals (#3 + dropping the stale module-level host). The full in-tree provider/portal rewrite (#1) and exported teardown handle (#2) remain follow-ups for a major release.

Changelog

New

  • Regression test under a useConfirm describe block asserting the host element is detached from <body> on close.

Changed

  • confirm() now creates a fresh host element per call and removes it from document.body after root.unmount():
const hostElement = document.createElement('div')
document.body.append(hostElement)
const root = createRoot(hostElement)
const onClose: ConfirmationDialogProps['onClose'] = gesture => {
  root.unmount()
  hostElement.remove()
  resolve(gesture === 'confirm')
}

Removed

  • Module-level let hostElement that was reused across calls and lingered on <body>.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Verify the new test fails on main (leaked host persists) and passes here. Behavior is unchanged on the happy path; only the orphaned container is now cleaned up. Worth confirming no consumer relied on scraping/reusing the persistent [role="alertdialog"] host.

Merge checklist

@changeset-bot

changeset-bot Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8d07b19

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copilot AI linked an issue Jun 8, 2026 that may be closed by this pull request
useConfirm renders into a detached createRoot that leaks to consumers (esp. tests) #7934
Open
@mattcosta7
Detach confirm() host element from body on close to fix leak
2765933 
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix useConfirm rendering issues with detached createRoot Fix useConfirm host element leaking into the DOM and tests Jun 8, 2026
Copilot AI requested a review from mattcosta7 June 8, 2026 15:50
@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

@mattcosta7

mattcosta7 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

this is a fix for now

ideally we wouldn't be creating a new react root for this, but we don't have a good path to avoid that currently

@mattcosta7 mattcosta7 marked this pull request as ready for review June 8, 2026 15:59
@mattcosta7 mattcosta7 requested a review from a team as a code owner June 8, 2026 15:59
@mattcosta7 mattcosta7 requested review from Copilot and siddharthkp June 8, 2026 15:59
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 8, 2026 16:02 Inactive
Copilot AI reviewed Jun 8, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a DOM leak in useConfirm() / confirm() where the detached createRoot() host <div> appended to document.body was never removed, accumulating empty containers and interfering with downstream consumers (notably RTL test cleanup).

Changes:

  • Remove the module-level reusable hostElement and create a fresh host element per confirm() call.
  • On dialog close, unmount the React root and remove the host element from document.body.
  • Add a regression test and a patch changeset documenting the behavior change.
Show a summary per file
File Description
packages/react/src/ConfirmationDialog/ConfirmationDialog.tsx Switch confirm() to per-call host creation and explicit host removal on close.
packages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsx Add regression test intended to ensure the confirm() host element is removed on close.
.changeset/confirm-dialog-host-cleanup.md Patch changeset describing the host cleanup behavior change.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread packages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsx Outdated
@mattcosta7 mattcosta7 requested a review from jonrohan June 9, 2026 16:04
@mattcosta7
Potential fix for pull request finding
87ab9c3 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 9, 2026 23:53 Inactive
@siddharthkp
Fix missing closing bracket and invalid matcher in ConfirmationDialog…
56cd164 
… test

Co-authored-by: siddharthkp <1863771+siddharthkp@users.noreply.github.com>
Copilot AI requested a review from siddharthkp June 10, 2026 08:55
@github-actions github-actions Bot requested a deployment to storybook-preview-7935 June 11, 2026 20:32 Abandoned
@mattcosta7 mattcosta7 added the Canary Release Apply this label when you want CI to create a canary release of the current PR label Jun 11, 2026
@primer-integration

primer-integration Bot commented Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Integration test results from github/github-ui PR:

Failed  CI   Failed
Running  VRT   Running
Passed  Projects   Passed

CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures.

Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance.

@mattcosta7 mattcosta7 enabled auto-merge June 12, 2026 11:39
@primer primer Bot disabled auto-merge June 12, 2026 15:31
@primer primer Bot enabled auto-merge June 12, 2026 15:31
@primer primer Bot disabled auto-merge June 12, 2026 15:35
@primer primer Bot enabled auto-merge June 12, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jonrohan jonrohan jonrohan approved these changes

@mattcosta7 mattcosta7 Awaiting requested review from mattcosta7

@siddharthkp siddharthkp Awaiting requested review from siddharthkp

Assignees

@mattcosta7 mattcosta7

Copilot code review Copilot

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

useConfirm renders into a detached createRoot that leaks to consumers (esp. tests)

5 participants

@mattcosta7 @jonrohan @siddharthkp